Add 'cluster', 'namespace', and 'user' options to delete command. Enhance delete to load provider components from cluster. #384

spew · 2018-06-22T18:19:15Z

What this PR does / why we need it:
This PR adds the 'cluster', 'namespace', and 'user' options to the clusterctl delete cluster command. These options match what is available in kubectl. Also, the delete cluster command is enhanced to load the provider components from cluster.

Release note:

NONE

@kubernetes/kube-deploy-reviewers

k8s-ci-robot · 2018-06-22T18:19:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [spew]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spew · 2018-06-22T19:54:36Z

/assign @roberthbailey @mkjelland @k4leung4

roberthbailey · 2018-06-22T21:25:43Z

clusterctl/clusterdeployer/clusterclient.go

@@ -65,15 +67,16 @@ func (c *clusterClient) removeKubeconfigFile() error {
 	return os.Remove(c.kubeconfigFile)
 }

-func NewClusterClientFromFile(kubeconfigFile string) (*clusterClient, error) {
-	c, err := clientcmd.NewClusterApiClientForDefaultSearchPath(kubeconfigFile)
+func NewClusterClientFromDefaultSearchPath(kubeconfigFile string, overrides tcmd.ConfigOverrides) (*clusterClient, error) {


This one treats the first param as a string and the prior treats it as the actual config? That seems backwards to me, but maybe that's because the libraries you are calling need to do the file handling....

Both versions of this function treat the kubeconfigFile argument as a string. With the addition of the ability to select a specific Context in the Kubeconfig search path, I chose to rename the method to more accurately reflect reality, namely, if an empty string is passed in for kubeconfigFile then when iclientcmd.NewClusterApiClientForDefaultSearchPath(...) is called it will search for all kubeconfigs in the default search path ($KUBECONFIG, ${HOME}/.kube/config, etc).

What do you think?

roberthbailey · 2018-06-22T21:26:09Z

clusterctl/clusterdeployer/clusterclient.go

@@ -65,15 +67,16 @@ func (c *clusterClient) removeKubeconfigFile() error {
 	return os.Remove(c.kubeconfigFile)
 }

-func NewClusterClientFromFile(kubeconfigFile string) (*clusterClient, error) {
-	c, err := clientcmd.NewClusterApiClientForDefaultSearchPath(kubeconfigFile)
+func NewClusterClientFromDefaultSearchPath(kubeconfigFile string, overrides tcmd.ConfigOverrides) (*clusterClient, error) {


I'm not sure I understand the name of this fn; why not name it NewFromFile(...)?

Please see my reply on the other comment about parameters and function name.

roberthbailey · 2018-06-22T21:26:51Z

clusterctl/clusterdeployer/clusterclient.go

+	clientSet       clientset.Interface
+	kubeconfigFile  string
+	configOverrides tcmd.ConfigOverrides
+	closeFn         func() error
 }

 func NewClusterClient(kubeconfig string) (*clusterClient, error) {


This should just be New(...) so that people who import the package call clusterclient.New(...) instead of clusterclient.NewClusterClient(...) which is redundantly long (for go style).

The reason why I think the original author named it NewClusterClient is because it lives in the clusterdeployer package which already has a New(...) method. Happy to move it to another package in another PR if you agree that is the thing to do (I think it probably is?)

Moving it in another PR would be great. Want to open an issue and label it as help-wanted? That would be a good thing for someone who wants to start sending code to take a crack at.

Created an issue here: #398

roberthbailey · 2018-06-22T21:27:53Z

clusterctl/clusterdeployer/clusterclient.go

@@ -65,15 +67,16 @@ func (c *clusterClient) removeKubeconfigFile() error {
 	return os.Remove(c.kubeconfigFile)


Does createTempFile handle removing the file automatically? I'm guessing not since we call os.Remove here. Which means that if there is an error creating the client we leak the file. We need to defer removing the file so that it gets cleaned up even on the error return paths.

I created a PR here: #390, which addresses this issue in two places. Note that there is still an opportunity to leak files if the user prematurely terminates the program or an automated system kills the VM / container. So in any sort of automated system we will need cleanup processes that handle leaked files.

PR #390 merged and I have rebased this commit on top.

roberthbailey · 2018-06-22T21:29:15Z

clusterctl/clusterdeployer/clusterclient.go

@@ -164,15 +167,31 @@ func (c *clusterClient) WaitForClusterV1alpha1Ready() error {

 func (c *clusterClient) kubectlApply(manifest string) error {
 	r := strings.NewReader(manifest)


I'm not sure why r is a separate variable instead of being inlined below (cmd.Stdin = strings.NewReader(manifest)).

Yeah, I should have changed this too in the refactor of the method. Will change.

roberthbailey · 2018-06-22T21:29:50Z

clusterctl/clusterdeployer/clusterclient.go

@@ -164,15 +167,31 @@ func (c *clusterClient) WaitForClusterV1alpha1Ready() error {

 func (c *clusterClient) kubectlApply(manifest string) error {
 	r := strings.NewReader(manifest)
-	cmd := exec.Command("kubectl", "apply", "--kubeconfig", c.kubeconfigFile, "-f", "-")
+	args := c.buildKubectlArgs("apply")


Unless we are going to print args or do something else with it, just put the call inline below

cmd := exec.Command("kubectl", c.buildKubectlArgs("apply")...)

Agree, changed.

roberthbailey · 2018-06-22T21:30:51Z

clusterctl/testdata/delete-cluster-no-args-invalid-flag.golden

@@ -3,8 +3,12 @@ Usage:
  clusterctl delete cluster [flags]

 Flags:
+      --cluster string               The name of the kubeconfig cluster to use


This help text isn't super clear without reading the code. Maybe "The name of the cluster to use in the kubeconfig file"?

We get this help text for 'free' from the libraries so it matches what kubectl has for help text (and actually the entire ecosystem of CLIs that are using the same client-go libraries). I do agree the message isn't super clear. However, there are some nice things about providing a consistent experience as users who are familiar with kubectl would know exactly what this means. So I see three options:

Improve the message and diverge from what kubectl prints out, open PR upstream in client-go to improve the kubectl string.

Leave message as it is (no custom code / message). Open PR to improve the messaging in client-go, wait for that to get vendored in.

Run with our own message and diverge completely from the rest of the kubernetes community.

Of these options I think I like #2 the best as it is the least work and the only users right now are early adopters. It will be less work because we don't have to remember to remove our custom message later once client-go picks up the new messaging. However, I'm happy to do #1 if you think that is best. What do you think? (or do you have another idea?).

Your second option seems reasonable to me.

Will follow up with another PR in kube core -- I have an open issue here: #397

roberthbailey · 2018-06-22T21:31:37Z

clusterctl/testdata/delete-cluster-no-args-invalid-flag.golden

  -h, --help                         help for cluster
+      --kubeconfig string            Path to the kubeconfig file to use for connecting to the cluster to be deleted, if empty, the default KUBECONFIG load path is used.
+  -n, --namespace string             If present, the namespace scope for this CLI request


This is namespace for the kubectl apply commands, which isn't super clear from the help text.

It is for kubectl apply true, but will also be the namespace for everything clusterctl delete cluster does as the clients will be configured for this namespace.

spew

Addressed a couple of issues and left open some questions for @roberthbailey before I go implement changes.

spew · 2018-06-23T12:18:24Z

clusterctl/clusterdeployer/clusterclient.go

+	clientSet       clientset.Interface
+	kubeconfigFile  string
+	configOverrides tcmd.ConfigOverrides
+	closeFn         func() error
 }

 func NewClusterClient(kubeconfig string) (*clusterClient, error) {


The reason why I think the original author named it NewClusterClient is because it lives in the clusterdeployer package which already has a New(...) method. Happy to move it to another package in another PR if you agree that is the thing to do (I think it probably is?)

spew · 2018-06-23T12:31:25Z

clusterctl/clusterdeployer/clusterclient.go

@@ -65,15 +67,16 @@ func (c *clusterClient) removeKubeconfigFile() error {
 	return os.Remove(c.kubeconfigFile)


I created a PR here: #390, which addresses this issue in two places. Note that there is still an opportunity to leak files if the user prematurely terminates the program or an automated system kills the VM / container. So in any sort of automated system we will need cleanup processes that handle leaked files.

spew · 2018-06-23T13:17:32Z

clusterctl/clusterdeployer/clusterclient.go

@@ -65,15 +67,16 @@ func (c *clusterClient) removeKubeconfigFile() error {
 	return os.Remove(c.kubeconfigFile)
 }

-func NewClusterClientFromFile(kubeconfigFile string) (*clusterClient, error) {
-	c, err := clientcmd.NewClusterApiClientForDefaultSearchPath(kubeconfigFile)
+func NewClusterClientFromDefaultSearchPath(kubeconfigFile string, overrides tcmd.ConfigOverrides) (*clusterClient, error) {


Both versions of this function treat the kubeconfigFile argument as a string. With the addition of the ability to select a specific Context in the Kubeconfig search path, I chose to rename the method to more accurately reflect reality, namely, if an empty string is passed in for kubeconfigFile then when iclientcmd.NewClusterApiClientForDefaultSearchPath(...) is called it will search for all kubeconfigs in the default search path ($KUBECONFIG, ${HOME}/.kube/config, etc).

What do you think?

spew · 2018-06-23T13:18:08Z

clusterctl/clusterdeployer/clusterclient.go

@@ -164,15 +167,31 @@ func (c *clusterClient) WaitForClusterV1alpha1Ready() error {

 func (c *clusterClient) kubectlApply(manifest string) error {
 	r := strings.NewReader(manifest)


Yeah, I should have changed this too in the refactor of the method. Will change.

spew · 2018-06-23T13:19:21Z

clusterctl/clusterdeployer/clusterclient.go

@@ -164,15 +167,31 @@ func (c *clusterClient) WaitForClusterV1alpha1Ready() error {

 func (c *clusterClient) kubectlApply(manifest string) error {
 	r := strings.NewReader(manifest)
-	cmd := exec.Command("kubectl", "apply", "--kubeconfig", c.kubeconfigFile, "-f", "-")
+	args := c.buildKubectlArgs("apply")


Agree, changed.

spew · 2018-06-23T13:24:30Z

clusterctl/testdata/delete-cluster-no-args-invalid-flag.golden

@@ -3,8 +3,12 @@ Usage:
  clusterctl delete cluster [flags]

 Flags:
+      --cluster string               The name of the kubeconfig cluster to use


We get this help text for 'free' from the libraries so it matches what kubectl has for help text (and actually the entire ecosystem of CLIs that are using the same client-go libraries). I do agree the message isn't super clear. However, there are some nice things about providing a consistent experience as users who are familiar with kubectl would know exactly what this means. So I see three options:

Improve the message and diverge from what kubectl prints out, open PR upstream in client-go to improve the kubectl string.

Leave message as it is (no custom code / message). Open PR to improve the messaging in client-go, wait for that to get vendored in.

Run with our own message and diverge completely from the rest of the kubernetes community.

Of these options I think I like #2 the best as it is the least work and the only users right now are early adopters. It will be less work because we don't have to remember to remove our custom message later once client-go picks up the new messaging. However, I'm happy to do #1 if you think that is best. What do you think? (or do you have another idea?).

spew · 2018-06-23T13:24:40Z

clusterctl/testdata/delete-cluster-no-args-invalid-flag.golden

  -h, --help                         help for cluster
+      --kubeconfig string            Path to the kubeconfig file to use for connecting to the cluster to be deleted, if empty, the default KUBECONFIG load path is used.
+  -n, --namespace string             If present, the namespace scope for this CLI request


It is for kubectl apply true, but will also be the namespace for everything clusterctl delete cluster does as the clients will be configured for this namespace.

k4leung4 · 2018-06-25T15:26:19Z

no additional beyond what @roberthbailey have pointed out.
lgtm

…ance delete to load provider components from cluster.

roberthbailey · 2018-06-25T21:09:58Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 22, 2018

k8s-ci-robot requested review from jessicaochen and krousey June 22, 2018 18:19

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 22, 2018

spew force-pushed the delete-cluster-params branch 2 times, most recently from eef7f3c to 03ca60d Compare June 22, 2018 18:43

k8s-ci-robot assigned k4leung4, mkjelland and roberthbailey Jun 22, 2018

spew force-pushed the delete-cluster-params branch 2 times, most recently from 84c2870 to 28a7872 Compare June 22, 2018 20:31

roberthbailey reviewed Jun 22, 2018

View reviewed changes

spew mentioned this pull request Jun 23, 2018

Fix an issue where clusterClient's temporary kubeconfig files could persist after the application terminates. Add comments for the clusterclient New(...) methods. #390

Merged

spew commented Jun 23, 2018

View reviewed changes

spew force-pushed the delete-cluster-params branch from 28a7872 to 5f5ce5b Compare June 23, 2018 13:30

Add 'cluster', 'namespace', and 'user' options to delete command. Enh…

056eddb

…ance delete to load provider components from cluster.

spew force-pushed the delete-cluster-params branch from 5f5ce5b to 056eddb Compare June 25, 2018 16:09

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2018

k8s-ci-robot merged commit 3784fb3 into kubernetes-sigs:master Jun 25, 2018

This was referenced Jun 25, 2018

Improve help text for 'kubeconfig', 'namespace', and 'cluster' in client-go #397

Closed

Move clusterclient to its own sub-package within clusterdeployer #398

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'cluster', 'namespace', and 'user' options to delete command. Enhance delete to load provider components from cluster. #384

Add 'cluster', 'namespace', and 'user' options to delete command. Enhance delete to load provider components from cluster. #384

spew commented Jun 22, 2018

k8s-ci-robot commented Jun 22, 2018

spew commented Jun 22, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018

roberthbailey Jun 25, 2018

spew Jun 25, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018

spew Jun 25, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018 •

edited

Loading

roberthbailey Jun 25, 2018

spew Jun 25, 2018

roberthbailey Jun 22, 2018

spew Jun 23, 2018

spew left a comment

spew Jun 23, 2018

spew Jun 23, 2018

spew Jun 23, 2018

spew Jun 23, 2018

spew Jun 23, 2018

spew Jun 23, 2018 •

edited

Loading

spew Jun 23, 2018

k4leung4 commented Jun 25, 2018

roberthbailey commented Jun 25, 2018

		@@ -65,15 +67,16 @@ func (c *clusterClient) removeKubeconfigFile() error {
		return os.Remove(c.kubeconfigFile)

		@@ -164,15 +167,31 @@ func (c *clusterClient) WaitForClusterV1alpha1Ready() error {

		func (c *clusterClient) kubectlApply(manifest string) error {
		r := strings.NewReader(manifest)

Add 'cluster', 'namespace', and 'user' options to delete command. Enhance delete to load provider components from cluster. #384

Add 'cluster', 'namespace', and 'user' options to delete command. Enhance delete to load provider components from cluster. #384

Conversation

spew commented Jun 22, 2018

k8s-ci-robot commented Jun 22, 2018

spew commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spew Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spew Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k4leung4 commented Jun 25, 2018

roberthbailey commented Jun 25, 2018

spew Jun 23, 2018 •

edited

Loading

spew Jun 23, 2018 •

edited

Loading